Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disallow hook definitions in tests #9957

Merged
merged 10 commits into from
May 4, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 3, 2020

Summary

Not breaking after #8096, just a cleaner error in this case.

Test plan

Test added

@codecov-io
Copy link

Codecov Report

Merging #9957 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9957   +/-   ##
=======================================
  Coverage   64.06%   64.07%           
=======================================
  Files         293      293           
  Lines       12457    12459    +2     
  Branches     3069     3070    +1     
=======================================
+ Hits         7981     7983    +2     
- Misses       3833     3834    +1     
+ Partials      643      642    -1     
Impacted Files Coverage Δ
packages/jest-circus/src/eventHandler.ts 6.00% <0.00%> (-0.13%) ⬇️
packages/expect/src/utils.ts 96.22% <0.00%> (+1.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e8f8d5...2983c35. Read the comment docs.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Leave it to you to merge pending @jeysal's input.

e2e/__tests__/nestedTestDefinitions.test.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/eventHandler.ts Outdated Show resolved Hide resolved
);
break;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushing into the errors array is essentially what happened before, except we don't go through the handler again into the error case

break;
} else if (hasStarted) {
state.unhandledErrors.push(
new Error(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to mutate the message in asyncError, this is sync code

@@ -214,7 +214,7 @@ export type TestError = Exception | [Exception | undefined, Exception]; // the e
export type TestEntry = {
type: 'test';
asyncError: Exception; // Used if the test failure contains no usable stack trace
errors: TestError;
errors: Array<TestError>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was any before... Array<any> is more accurate, but still not particularly good 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, now it can be an array of tuples? Look at TestError

Copy link
Member Author

@SimenB SimenB May 4, 2020

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can see it used in formatNodeAssertErrors.ts

@SimenB SimenB merged commit d30a586 into jestjs:master May 4, 2020
@SimenB SimenB deleted the disallow-hooks-in-tests branch May 4, 2020 15:49
@@ -16,6 +16,7 @@ FAIL __tests__/asyncDefinition.test.js
14 | });
15 | });

at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be stripped out from stack trace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, it was painful 😀 We could copy the stack from asyncError I guess

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants